✨amp-form: Initialize form from URL #24671

mattwomple · 2019-09-21T04:11:28Z

Introduce amp-form attribute data-initialize-from-url and form field
attribute data-allow-initialization to selectively populate form
fields from URL query parameter values on page load.

Fixes #24533
Fixes #24876

mattwomple · 2019-09-21T04:16:09Z

@ampproject/wg-ui-and-a11y I understand this isn't ready for a complete review, but any guidance you can provide on the TODO items in the summary is appreciated.

caroqliu · 2019-10-01T16:19:46Z

Nice initiative developing the TODOs! Some comments:

Regarding adding initializeFromUrl in the constructor, I'd recommend taking a look at doing this from the layoutCallback
Add a check for a11y testing this interaction pattern. For example, will SR users be notified that inputs are filled with publisher-provided values?
In addition to checking targeted input types, consider verifying behavior with undesirable input types, including usages that go against AMP validator rules.

Before spending more time on implementation, I recommend having further conversation with regards to design. Have you considered filing an intent-to-implement (I2I)?

amp-owners-bot · 2019-10-21T17:26:17Z

Hey @ampproject/wg-caching, these files were changed:

validator/validator-main.protoascii

mattwomple · 2019-10-21T17:31:03Z

Thanks for the feedback @caroqliu . Regarding your fist suggestion, to move initializeFromUrl() into unlayoutCallback(), is this possible with amp-form? This extension doesn't create a custom element like most other extensions; the HTML on the page is just <form>. The class is defined with just export class AmpForm {...} instead of export class AmpForm extends AMP.BaseElement {...}. I might misunderstand, but I'm under the impression that the layout lifecycle only applies to custom elements.

Commit efdb8f4:

removes the ability to update checkbox and radio inputs
does not interfere with data-amp-replace inputs

extensions/amp-form/0.1/amp-form.js

caroqliu · 2019-10-21T18:05:17Z

Regarding your fist suggestion, to move initializeFromUrl() into unlayoutCallback(), is this possible with amp-form?

You're right, disregard this suggestion 😄

caroqliu

Saw you signed up for design review! The I2I had some discussion about content jumping--it would be useful to nail that topic down in your review.

extensions/amp-form/0.1/amp-form.js

mattwomple · 2019-10-21T20:38:38Z

Saw you signed up for design review! The I2I had some discussion about content jumping--it would be useful to nail that topic down in your review.

Right. I hope that I've removed this worry by omitting checkbox and radio inputs from those that can be handled by this feature. I look forward to discussing it in design review.

Introduce amp-form attribute `data-initialize-from-url` and form field attribute `data-allow-initialization` to selectively populate form fields from URL query parameter values on page load. Fixes #24533

extensions/amp-form/amp-form.md

extensions/amp-form/0.1/test/test-amp-form.js

extensions/amp-form/0.1/amp-form.js

caroqliu · 2019-11-08T16:51:46Z

extensions/amp-form/0.1/amp-form.js

+      const value = queryParams[key] || '';
+      const type = field.getAttribute('type') || 'text';
+      const tag = field.tagName;
+


What do you think about using isFieldDefault and isFieldEmpty methods from the src/form to early return if the user has already interacted with the input? That way we also don't have to worry about the field value checks before setting them. This suggestion assumes that the form would not want to change/clear any user progress that has been made.

My original intention with the field value checks was to determine whether to trigger a form update event after all fields were updated. Since that feature is removed (we settled on the form being considered clean instead of dirty after populating from URL), I suppose these checks are not strictly necessary. I still feel uneasy about setting .value when unnecessary (thinking about additional listeners that may be attached to inputs in the future), so I tend to like the field value checks.

W.r.t isFieldDefault and isFieldEmpty, do you think it's a significant concern that a user could modify a form before amp-form.js loads and the constructor runs?

Based on how the generic form dirtiness check is implemented it looks like it is really only a concern at initialization if amp-bind has modified the inputs but not if the user has. In that case I think this is fine as is.

I appreciate you taking the time to look through the PR closely. Thanks much for the review.

dreamofabear · 2019-11-11T16:04:09Z

extensions/amp-form/0.1/amp-form.js

+
+    const queryParams = parseQueryString(this.win_.location.search);
+    Object.keys(queryParams).forEach(key => {
+      const field = this.form_./*OK*/ querySelector(`[name="${key}"]`);


Can we use this.form_.elements[key] here? It would also support non-descendant fields that reference the form by name and might even be a bit faster.

We don't do this consistently in existing amp-form code unfortunately.

I like this suggestion, but might need a little help understanding where/why check-types fails.

form.elements[key] can return a NodeList (for radio inputs) or an Element. I took this into account with my refactor of the maybeInitializeFromUrl_() function. See gist. Now, the check-types test fails:

Type checking failed: extensions/amp-form/0.1/amp-form.js:1297: ERROR - [JSC_TYPE_MISMATCH] actual parameter 1 of iterateCursor$$module$src$dom does not match formal parameter found : HTMLElement required: IArrayLike<?> missing : [length] mismatch: [] iterateCursor(fields, field => maybeFillField(field, key));

Does this indicate that the check-types test needs to be updated to understand that form.elements[key] can return a NodeList? Or is there a way that I can tell the test "hey, i know this is iterable"?

Ah, the Closure type for HTMLFormElement.elements might be incorrect (missing the list case). Typically we typecast to workaround.

if (fields.length) { // Typecast since Closure is missing NodeList union type in HTMLFormElement.elements. const list = /** @type {!NodeList} */ (fields); [...] }

Sounds reasonable, but not allowed:

extensions/amp-form/0.1/amp-form.js:1307: ERROR - [JSC_INVALID_CAST] invalid cast - must be a subtype or supertype from: HTMLElement to : NodeList const list = /** @type {!NodeList} */ (field); ^^^^^^^

Ok, I think I have a handle on this:

// Typecast since Closure is missing NodeList union type in HTMLFormElement.elements. const field = /** @type {(!NodeList|!Element)} */ (this.form_.elements[key]);

This should be resolved in 1894b76. Thanks much for the review and assistance!

dreamofabear · 2019-11-11T16:06:51Z

extensions/amp-form/0.1/amp-form.js

+        if (field.value !== value) {
+          field.value = value;
+        }
+      } else if (tag === 'INPUT' && checkedInputTypes.includes(type)) {


Nit: Extract duplicate tag === 'INPUT' check into an outer conditional.

This should be resolved in 1894b76.

dreamofabear · 2019-11-11T16:09:30Z

extensions/amp-form/amp-form.md

@@ -129,6 +129,30 @@ The value for `action-xhr` can be the same or a different endpoint than `action`

 To learn about redirecting the user after successfully submitting the form, see the [Redirecting after a submission](#redirecting-after-a-submission) section below.

+##### data-initialize-from-url
+
+Initializes supported form fields from the URL. When this attribute is present, `<input>`, `<select>`, and `<textarea>` fields under the form can optionally be initialized from URL query parameter values.


Suggestion: "Initializes input fields from the document URL's search string, where the query parameter name matches the field's name."

I'm ok with modifying this, but I'm not sure your suggestion is the right final text.

This is the only place where the supported form field tags are written: <input>, <select>, and <textarea>.

From the document URL makes it sound like the URL of the AMP document matters more than window.location in a PWA. This is not the case.

I've been trying to avoid using the word "input" when describing form fields because <input> is just one of three supported form fields (or form controls, which would be more accurate but less recognizable by readers).

Ah, I just meant to supplement the first sentence. Window URL and "form field" sound fine.

This should be resolved in 1894b76.

extensions/amp-form/0.1/amp-form.js

- Prefer form.element['key'] to form.querySelector('[name=key]') - Refactor if/else to extract tag === 'INPUT' conditional - Revise summary description of data-initialize-from-url in readme

mattwomple · 2019-11-16T19:48:34Z

Thank you for the reviews everyone. Are there others that should be given a chance to review this before it is green-lighted?

Gregable · 2019-11-19T22:25:11Z

@choumx @honeybadgerdontcare , is this ready to submit, or do you feel we need another review. If the former, will one of you merge?

dreamofabear · 2019-11-19T22:29:46Z

LGTM. I'd like to let @mattwomple merge since he can. :)

Gregable · 2019-11-19T22:44:03Z

Ah, perfect. I didn't know he had merge power. I'm usually confused on this point, sorry Matt.

* ✨amp-form: Initialize form from URL Introduce amp-form attribute `data-initialize-from-url` and form field attribute `data-allow-initialization` to selectively populate form fields from URL query parameter values on page load. Fixes ampproject#24533 * Documentation tweaks * Revisions based on self-review * Revisions from review - Prefer form.element['key'] to form.querySelector('[name=key]') - Refactor if/else to extract tag === 'INPUT' conditional - Revise summary description of data-initialize-from-url in readme

googlebot added the cla: yes label Sep 21, 2019

mattwomple mentioned this pull request Sep 30, 2019

Allow form element default values to be set from URL parameters #24533

Closed

mattwomple mentioned this pull request Oct 3, 2019

I2I: amp-form: Initialize form from URL #24876

Closed

honeybadgerdontcare approved these changes Oct 21, 2019

View reviewed changes

caroqliu reviewed Oct 21, 2019

View reviewed changes

extensions/amp-form/0.1/amp-form.js Outdated Show resolved Hide resolved

mattwomple mentioned this pull request Oct 21, 2019

Design Review 2019-10-23 20:00 UTC (initialize form inputs from query, CSI migration, deprecate recursive form submission) #24591

Closed

caroqliu reviewed Oct 21, 2019

View reviewed changes

extensions/amp-form/0.1/amp-form.js Outdated Show resolved Hide resolved

✨amp-form: Initialize form from URL

56f93cf

Introduce amp-form attribute `data-initialize-from-url` and form field attribute `data-allow-initialization` to selectively populate form fields from URL query parameter values on page load. Fixes #24533

mattwomple changed the title ~~[WIP] ✨amp-form: Initialize form from URL~~ ✨amp-form: Initialize form from URL Nov 4, 2019

mattwomple marked this pull request as ready for review November 4, 2019 00:37

Documentation tweaks

3090926

mattwomple commented Nov 4, 2019

View reviewed changes

Revisions based on self-review

f7f1353

mattwomple requested a review from caroqliu November 5, 2019 06:41

dreamofabear self-requested a review November 6, 2019 16:53

caroqliu reviewed Nov 8, 2019

View reviewed changes

caroqliu approved these changes Nov 8, 2019

View reviewed changes

dreamofabear approved these changes Nov 11, 2019

View reviewed changes

Revisions from review

1894b76

- Prefer form.element['key'] to form.querySelector('[name=key]') - Refactor if/else to extract tag === 'INPUT' conditional - Revise summary description of data-initialize-from-url in readme

mattwomple merged commit a945239 into ampproject:master Nov 19, 2019

mattwomple deleted the patch-1 branch November 19, 2019 22:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨amp-form: Initialize form from URL #24671

✨amp-form: Initialize form from URL #24671

mattwomple commented Sep 21, 2019 •

edited

Loading

mattwomple commented Sep 21, 2019

caroqliu commented Oct 1, 2019

amp-owners-bot bot commented Oct 21, 2019

mattwomple commented Oct 21, 2019

caroqliu commented Oct 21, 2019

caroqliu left a comment

mattwomple commented Oct 21, 2019

caroqliu Nov 8, 2019

mattwomple Nov 8, 2019

caroqliu Nov 8, 2019

mattwomple Nov 8, 2019

dreamofabear Nov 11, 2019

mattwomple Nov 11, 2019

dreamofabear Nov 11, 2019

mattwomple Nov 11, 2019

mattwomple Nov 11, 2019

mattwomple Nov 11, 2019

dreamofabear Nov 11, 2019

mattwomple Nov 11, 2019

dreamofabear Nov 11, 2019

mattwomple Nov 11, 2019

dreamofabear Nov 11, 2019

mattwomple Nov 11, 2019

mattwomple commented Nov 16, 2019

Gregable commented Nov 19, 2019

dreamofabear commented Nov 19, 2019

Gregable commented Nov 19, 2019

✨amp-form: Initialize form from URL #24671

✨amp-form: Initialize form from URL #24671

Conversation

mattwomple commented Sep 21, 2019 • edited Loading

mattwomple commented Sep 21, 2019

caroqliu commented Oct 1, 2019

amp-owners-bot bot commented Oct 21, 2019

mattwomple commented Oct 21, 2019

caroqliu commented Oct 21, 2019

caroqliu left a comment

Choose a reason for hiding this comment

mattwomple commented Oct 21, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattwomple commented Nov 16, 2019

Gregable commented Nov 19, 2019

dreamofabear commented Nov 19, 2019

Gregable commented Nov 19, 2019

mattwomple commented Sep 21, 2019 •

edited

Loading